Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Store remote channel announcement information into wallet DB #2466

Closed
wants to merge 1 commit into from

Conversation

trueptolemy
Copy link
Collaborator

@trueptolemy trueptolemy commented Mar 12, 2019

This PR draft tries to fix issue #2409.

We don't store the announcement signatures before, so when restart we may waste much time on waiting for remote announcement reply. So this PR tries to store the remote announcement signatures into DB like what @SimonVrouwe proposed (we don't store local announcement signatures because we can drive sigs locally).

Instead of adding a new wire type between Channeld and MASTER, I change the wire_channel_got_funding_locked and ask it not only tell MASTER remote funding locked, but also send remote announcement sigs to MASTER and store sigs.

There are 4 changes:

1. Rename wire_channel_got_funding_locked to wire_channel_got_locked_and_announced, and add announcement data into the message of this type.

Now the message will be like this:
WIRE_CHANNEL_GOT_LOCKED_AND_ANNOUNCEMENT + got_remote_announcement(bool) + next_per_commit_point(pubkey) + remote_announcement_node_sigs(signature) + remote_announcement_bitcoin_sigs(signature)
The bool type above means if the message is to tell MASTER announcement or not(instead, to tell MASTER remote funding locked).

There 2 situations that this message will be sent:

  • When Channeld receive remote peer funding_locked message, it will tell MASTER funding got locked remote. We will set the message like this:
    msg: WIRE_CHANNEL_GOT_LOCKED_AND_ANNOUNCEMENT + false + next_per_commit_point(pubkey) + NULL + NULL
    In this case, the msg equals the original wire_channel_got_funding_locked.
  • When Channeld receive remote peer announcement message, it will tell MASTER the announcement signatures. We will the set message like this:
    msg: WIRE_CHANNEL_GOT_LOCKED_AND_ANNOUNCEMENT + true + NULL + remote_announcement_node_sigs + remote_announcement_bitcoin_sigs
    Channeld will check if we received the same announcement before and ignore the same announcement.
    Channeld only send wire_channel_got_locked_and_announced for announcement when it receives announcement for the first time.

2. Add announcement struct in channel struct (in lightningd/channeld.h).

struct announcement {
	secp256k1_ecdsa_signature announcement_node_sigs;
	secp256k1_ecdsa_signature announcement_bitcoin_sigs;
};

When MASTER receive announcement signatures, we will put signatures in this struct and store it into DB.

3. Add announcement table in DB, and add wallet_announcement_save() and
wallet_announcement_load() function.

  • announcement table:
    The announcement table store data with the type like: (channelid, remote_node_sigs, remote_bitcoin_sigs).
    When we create a new channel and insert channel into DB, the announcement data will be set. If channel doesn't set ANNOUNCEMENT flag or hasn't received remote announcement message, we will set (channelid, NULL, NULL) for this channel.
  • wallet_announcement_save():
    This function will only be called when MASTER receive wire_channel_got_locked_and_announced with flag setting true and resolve announcement signatures into announcement struct.
  • wallet_announcement_load():
    This function will only be called when we initial a channel from DB (this will happen when restart).

4. Add announcement save and load test in wallet/test/run_wallet.c.

Compared to adding a new announcement wire type between Channeld and Lightningd, this way isn't so direct.
Anyway, it's just a try. What do you think about?

@trueptolemy trueptolemy force-pushed the issue-2409 branch 5 times, most recently from 2b584f4 to 24eb994 Compare March 17, 2019 15:30
@rustyrussell
Copy link
Contributor

Note that this is stuck waiting for #2405 to be rebased?

@trueptolemy
Copy link
Collaborator Author

trueptolemy commented Apr 3, 2019

@rustyrussell Thanks for your reply! This PR has nothing to do with the rebase of #2405 .
My main idea here is to modify wire_channel_got_funding_locked, and let this wire message not only tell MASTER remote funding locked, but also send remote announcement sigs to MASTER and store sigs:

  1. Rename wire_channel_got_funding_locked to wire_channel_got_locked_and_announced, and add announcement data into the message of this type.

Now the message will be like this:
WIRE_CHANNEL_GOT_LOCKED_AND_ANNOUNCEMENT + got_remote_announcement(bool) + next_per_commit_point(pubkey) + remote_announcement_node_sigs(signature) + remote_announcement_bitcoin_sigs(signature)
The bool type above means if the message is to tell MASTER announcement or not(instead, to tell MASTER remote funding locked).

There 2 situations that this message will be sent:

When Channeld receive remote peer funding_locked message, it will tell MASTER funding got locked remote. We will set the message like this:
msg: WIRE_CHANNEL_GOT_LOCKED_AND_ANNOUNCEMENT + false + next_per_commit_point(pubkey) + NULL + NULL
In this case, the msg equals the original wire_channel_got_funding_locked.
When Channeld receive remote peer announcement message, it will tell MASTER the announcement signatures. We will the set message like this:
msg: WIRE_CHANNEL_GOT_LOCKED_AND_ANNOUNCEMENT + true + NULL + remote_announcement_node_sigs + remote_announcement_bitcoin_sigs
Channeld will check if we received the same announcement before and ignore the same announcement.
Channeld only send wire_channel_got_locked_and_announced for announcement when it receives announcement for the first time.

In fact, I'm not sure if this trick is appropriate?

@trueptolemy trueptolemy force-pushed the issue-2409 branch 2 times, most recently from 3ee8d1b to 07f8fd8 Compare April 8, 2019 08:40
@trueptolemy trueptolemy changed the title Fix: Store remote announcement information into wallet DB Fix: Store remote channel announcement information into wallet DB Apr 8, 2019
1. Rename wire_channel_got_funding_locked to wire_channel_got_locked_and
   _announced, and add announcement data into the message of this type.
2. Add announcement struct in channel struct (in lightningd/channeld.h);
3. Add announcement table in DB, and add wallet_announcement_save() and
   wallet_announcement_load() function.
4. Add announcement save and load test.
@cdecker
Copy link
Member

cdecker commented Apr 10, 2019

I would have actually liked a new message rather than piggy-backing this onto an existing message. Having messages that serve multiple purposes is really weird, and likely better served in separate handlers.

It would have also been better to just add the two fields to the channels table since there is a 1-to-1 relationship to that.

@trueptolemy
Copy link
Collaborator Author

@cdecker Thank you for your very useful advice!! I'll modify it these days!

@trueptolemy trueptolemy deleted the issue-2409 branch May 4, 2019 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants